feat(cache): Add `Cache-Control: no-cache` support for Loki instant queries. #12896

kavirajk · 2024-05-06T09:31:41Z

What this PR does / why we need it:
Goal is, if header Cache-Control: no-cache is set, we guarantee that response user got for that query is never from the results cache.

Two changes at high-level

Loki: Queryfrontend interprets Cache-Control header and bypass the results if the value is no-cache.
LogCLI: Add extra flag --nocache to support setting the right header when sending queries to Loki.

Changes in Action:

Normal case using results cache.

-bash-5.2$ ./cmd/logcli/logcli instant-query 'sum(rate({job="varlogs"}[3h]))' -q
[
  {
    "metric": {},
    "value": [
      1715602199.729,
      "0.035555555555555556"
    ]
  }

Hitting the results cache from the following log lines in Loki

cache_result_req=17 cache_result_hit=16

Using HTTP header to bypass results caching

-bash-5.2$ ./cmd/logcli/logcli instant-query 'sum(rate({job="varlogs"}[3h]))' --nocache -q
[
  {
    "metric": {},
    "value": [
      1715602387.564,
      "0.035555555555555556"
    ]
  }

Not hitting the results cache from the following logs in Loki

cache_result_req=0 cache_result_hit=0

Which issue(s) this PR fixes:
Fixes NA

Special notes for your reviewer:

This PR adds support only for instant queries now. Follow up PRs will support for all types of queries.
Instant queries uses downstream engine via split_by_range middleware. Which converts LokiInstantRequest to Params and convert back from Params to LokiInstantRequest. So added support to persist cachingOptions in params as well.
Since this header is only valuable for Query-frontend, we don't persist this header when passing the subqueries to the querier (via HTTP -> GRPC -> HTTP roundtrip).

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
Title matches the required conventional commits format, see here
- Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR
If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

Goal is, if that header is set, we gurantee that response user got for that query is never from results cache. Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

ashwanthgoli

lgtm. just a couple of nits

ashwanthgoli · 2024-05-14T13:26:58Z

pkg/storage/chunk/cache/resultscache/cache_test.go

+	rc.put(ctx, key, []Extent{mkExtent(int64(modelNow)-(600*1e3), int64(modelNow))})
+
+	// Asserts (when `shouldCacheReq` is non-nil and set to return false (should not cache), resultcache should get result from upstream handler (mockHandler))
+	// 1. With `shouldCacheReq` non-nil and set `noCacheReq`, should get result from `next` handler


should we change this shouldCacheReq to shouldLookupCache or bypassCache?
the current name is a bit confusing, it reads "whether this request should be cached"?

Agree. I can change it in my tests. But the original type still called ShouldCacheReq (IMO, still confusing, because the same request's results can still be cached after getting it from the upstream).

Should ideally be called something like ShouldLookupCache. I will fix the naming on my test now. Renaming the original type I can do it in separate PR may be.

ashwanthgoli · 2024-05-14T13:30:53Z

pkg/storage/chunk/cache/resultscache/cache_test.go

+
+	// fill cache
+	key := ConstSplitter(day).GenerateCacheKey(context.Background(), "1", req)
+	rc.put(ctx, key, []Extent{mkExtent(int64(modelNow)-(600*1e3), int64(modelNow))})


this could be flaky at times, because the extent might end up in the previous day when this test runs on a day boundary

we could use constant time range similar to other tests to keep this predictable? mkExtent(50, 120)

extent should be irrelevent here I think because, the test is make sure we are calling upstream next handler instead of looking up in the cache in the first place (independent of cache hit or cache miss).

But good idea to make it constant anyway. Will update it.

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

pull-request-size bot added the size/S label May 6, 2024

kavirajk force-pushed the kavirajk/aggregation-instant-query branch from 40eb3ed to 1b77ecf Compare May 6, 2024 12:10

kavirajk added 2 commits May 13, 2024 08:43

Add support for Cache-Control: no-cache support for Loki queries.

1328ee5

Goal is, if that header is set, we gurantee that response user got for that query is never from results cache. Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

fix codec setting cache disabled flag

12a591a

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

kavirajk force-pushed the kavirajk/aggregation-instant-query branch from 8ad5cd3 to 12a591a Compare May 13, 2024 06:43

pass on CachingOptions to downstream engine

596a8bc

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

pull-request-size bot added size/M and removed size/S labels May 13, 2024

kavirajk changed the title ~~feat(cache): Add support for Cache-Control: no-cache support for Loki queries.~~ feat(cache): Add Cache-Control: no-cache support for Loki instant queries. May 13, 2024

kavirajk added 2 commits May 13, 2024 14:07

cleanup variables and constants

b356ec5

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Add tests

71c18cf

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

pull-request-size bot added size/L and removed size/M labels May 14, 2024

kavirajk marked this pull request as ready for review May 14, 2024 07:06

kavirajk requested a review from a team as a code owner May 14, 2024 07:06

kavirajk added 2 commits May 14, 2024 09:06

Merge branch 'main' into kavirajk/aggregation-instant-query

6d3f1ad

make linter happy

69fbdef

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

ashwanthgoli approved these changes May 14, 2024

View reviewed changes

kavirajk added 4 commits May 16, 2024 11:21

PR remarks

ebdbc86

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Merge branch 'main' into kavirajk/aggregation-instant-query

8a57330

fix paramsDetectedLabelsWrapper

9867e6c

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>

Merge branch 'main' into kavirajk/aggregation-instant-query

068a31e

kavirajk merged commit 88e545f into main May 19, 2024
58 checks passed

kavirajk deleted the kavirajk/aggregation-instant-query branch May 19, 2024 15:40

loki-gh-app bot mentioned this pull request May 20, 2024

chore(k203): release 3.1.0 #12988

Closed

This was referenced May 27, 2024

chore(k204): release 3.1.0 #13037

Closed

chore(k205): release 3.1.0 #13102

Closed

This was referenced Jun 10, 2024

chore(k206): release 3.1.0 #13184

Closed

chore(k207): release 3.1.0 #13225

Merged

loki-gh-app bot mentioned this pull request Jun 24, 2024

chore(k208): release 3.1.0 #13291

Closed

loki-gh-app bot mentioned this pull request Jul 1, 2024

chore(k209): release 3.1.0 #13356

Closed

grafanabot mentioned this pull request Jul 2, 2024

chore: [main] chore(k207): release 3.1.0 #13391

Open

This was referenced Jul 3, 2024

chore(release-3.1.x): release 3.0.1 #13402

Closed

chore(k210): release 3.1.0 #13435

Closed

chore(k210): release 3.1.0 #13462

Closed

RodrigoCMoraes mentioned this pull request Jul 12, 2024

incognia inloco/loki#22

Closed

loki-gh-app bot mentioned this pull request Jul 15, 2024

chore(k211): release 3.1.0 #13521

Closed

loki-gh-app bot mentioned this pull request Jul 22, 2024

chore(k212): release 3.1.0 #13595

Closed

loki-gh-app bot mentioned this pull request Oct 18, 2024

chore(release-3.1.x): release 3.1.3 #14531

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cache): Add `Cache-Control: no-cache` support for Loki instant queries. #12896

feat(cache): Add `Cache-Control: no-cache` support for Loki instant queries. #12896

kavirajk commented May 6, 2024 •

edited

Loading

ashwanthgoli left a comment

ashwanthgoli May 14, 2024

kavirajk May 16, 2024

ashwanthgoli May 14, 2024

ashwanthgoli May 14, 2024

kavirajk May 16, 2024

feat(cache): Add Cache-Control: no-cache support for Loki instant queries. #12896

feat(cache): Add Cache-Control: no-cache support for Loki instant queries. #12896

Conversation

kavirajk commented May 6, 2024 • edited Loading

ashwanthgoli left a comment

Choose a reason for hiding this comment

ashwanthgoli May 14, 2024

Choose a reason for hiding this comment

kavirajk May 16, 2024

Choose a reason for hiding this comment

ashwanthgoli May 14, 2024

Choose a reason for hiding this comment

ashwanthgoli May 14, 2024

Choose a reason for hiding this comment

kavirajk May 16, 2024

Choose a reason for hiding this comment

feat(cache): Add `Cache-Control: no-cache` support for Loki instant queries. #12896

feat(cache): Add `Cache-Control: no-cache` support for Loki instant queries. #12896

kavirajk commented May 6, 2024 •

edited

Loading